Skip to content

fix(mcp): JSON-serialize order_by_cols and support sort direction#39952

Open
scouredimage wants to merge 1 commit intoapache:masterfrom
scouredimage:fix/mcp-order-by-cols-serialization
Open

fix(mcp): JSON-serialize order_by_cols and support sort direction#39952
scouredimage wants to merge 1 commit intoapache:masterfrom
scouredimage:fix/mcp-order-by-cols-serialization

Conversation

@scouredimage
Copy link
Copy Markdown

@scouredimage scouredimage commented May 7, 2026 •
edited
Loading

SUMMARY

map_table_config in chart_utils.py currently passes config.sort_by (a list of bare column-name strings) straight through to the saved form_data["order_by_cols"]. The frontend (packages/superset-ui-core/src/query/extract/extractQueryFields.ts) expects each entry in order_by_cols to be a JSON-serialized [column, ascending] tuple and calls JSON.parse() on each item — bare strings cause a SyntaxError and the chart fails to render.

This PR fixes the serialization and additionally exposes sort direction to MCP callers, since the underlying form_data shape supports it but the schema didn't.

Changes

  • New SortByConfig model with an explicit ascending: bool (default False).
  • TableChartConfig.sort_by now accepts List[str | SortByConfig]. Bare strings default to descending — the natural choice for the sort-by-metric "top N" pattern most common for tables.
  • map_table_config JSON-serializes each entry as [column, ascending].

BEFORE/AFTER

# config.sort_by = ["product", "revenue"]

# before
form_data["order_by_cols"] == ["product", "revenue"]
# → frontend JSON.parse("product") raises SyntaxError

# after
form_data["order_by_cols"] == ['["product", false]', '["revenue", false]']

# new: explicit direction
config.sort_by = [
    SortByConfig(column="product", ascending=True),
    "revenue",  # still works, defaults to desc
]
form_data["order_by_cols"] == ['["product", true]', '["revenue", false]']

TESTING INSTRUCTIONS

pytest tests/unit_tests/mcp_service/chart/test_chart_utils.py::TestMapTableConfig -v

The existing test_map_table_config_with_sort is updated for the new format, and a new test_map_table_config_with_sort_direction covers the SortByConfig path and the mixed string/object case.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
  • Introduces new feature or API
  • Removes existing feature or API

@bito-code-review
Copy link
Copy Markdown
Contributor

bito-code-review Bot commented May 7, 2026 •
edited
Loading

Code Review Agent Run #95b19f

Actionable Suggestions - 0
Review Details
  • Files reviewed - 2 · Commit Range: 40acca7..40acca7
    • superset/mcp_service/chart/chart_utils.py
    • tests/unit_tests/mcp_service/chart/test_chart_utils.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@dosubot dosubot Bot added the change:backend Requires changing the backend label May 7, 2026
@scouredimage
fix(mcp): JSON-serialize order_by_cols and support sort direction
efee1ac 
The frontend's extractQueryFields.ts expects each entry in order_by_cols
to be a JSON-serialized [column, ascending] tuple and calls JSON.parse()
on each item. Bare strings caused a parse error and the chart failed
to render.

This commit:

- Adds a SortByConfig schema with explicit `ascending` direction
- Extends TableChartConfig.sort_by to accept either a column-name string
  (defaults to descending — the typical sort-by-metric "top N" pattern)
  or a SortByConfig object
- JSON-serializes each entry as [column, ascending] in map_table_config
@scouredimage scouredimage force-pushed the fix/mcp-order-by-cols-serialization branch from 40acca7 to efee1ac Compare May 7, 2026 19:56
@pull-request-size pull-request-size Bot added size/M and removed size/XS labels May 7, 2026
@scouredimage scouredimage changed the title fix(mcp): JSON-serialize order_by_cols as [col, ascending] tuples fix(mcp): JSON-serialize order_by_cols and support sort direction May 7, 2026
aminghadersohi
aminghadersohi approved these changes May 7, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@aminghadersohi aminghadersohi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug fix is correct — verified against the frontend source at extractQueryFields.ts:115-119, which documents the expected format explicitly in a comment and calls JSON.parse() on each order_by_cols entry. Bare column-name strings cause a SyntaxError there; JSON-serialized [col, bool] tuples are the correct shape.

SortByConfig is a clean addition — validation is tight, the alias (col) and default direction are documented, and the mixed-type union works properly via Pydantic's discriminated parsing.

Tests cover all three paths (bare string, explicit SortByConfig, mixed). Bito confirmed MyPy and Ruff clean.

One minor style note for future PRs: the project's CLAUDE.md prefers lowercase list[str] (Python 3.10+ style) over List[str] from typing — worth adopting on new code even when the surrounding file hasn't been migrated yet.

@bito-code-review
Copy link
Copy Markdown
Contributor

bito-code-review Bot commented May 8, 2026 •
edited
Loading

Code Review Agent Run #ed7b4c

Actionable Suggestions - 0
Review Details
  • Files reviewed - 3 · Commit Range: efee1ac..efee1ac
    • superset/mcp_service/chart/chart_utils.py
    • superset/mcp_service/chart/schemas.py
    • tests/unit_tests/mcp_service/chart/test_chart_utils.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aminghadersohi aminghadersohi aminghadersohi approved these changes

Assignees

No one assigned

Labels

change:backend Requires changing the backend size/M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@scouredimage @aminghadersohi